Improve DKIM support #485

yanokwa · 2023-09-17T22:21:53Z

DKIM is essentially a requirement to get emails delivered, so my high-level goal is to make it easier to turn on, without having to risk merge conflicts. Some things that you should know about the SMTP container.

It needs to have an existing file or the volume mapping will create an empty folder. That's why rsa.empty is there as a place holder in the compose file. It prevent that from happening.
It expects the rsa.private file at /etc/exim4/dkim.key.temp so that's why that's hard coded in the compose file.
It enables DKIM when DKIM_KEY_PATH has a value, so by default, we set it to empty, via DKIM_CONTAINER_KEY to disable it. Only when DKIM_CONTAINER_KEY has a value will DKIM be enabled. And the container expects this value to be the same as the container location of the mapped file, so that's why the .env has that value hard coded.
config.disabled can be removed because those rules are now baked into the container.

I don't love how confusing this is, but it's the best I could think of. Open to feedback.

What has been done to verify that this works as intended?

I confirmed that, after rebuild, emails send after removing rsa.private and rsa.public files and with the DKIM_* in the env file commented out. The emails are not DKIM enabled when they are received.

I've also confirmed, after rebuild, that emails send after adding rsa.private and rsa.public files and with the DKIM_* enabled. The emails are DKIM enabled.

Why is this the best possible solution? Were any other approaches considered?

I considered just changing the permissions on rsa.private and leaving our implementation as is, but that's not as secure.

It could be nicer if the upstream container generated the keys. That'd simplify things, but it feels out of scope.

Honestly, we should consider removing this SMTP container entirely and ask that people use an upstream mail service. That's what Discourse, Ghost, etc, do.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

If you already have DKIM support, all you need to do is add values to .env and it should now work because the permissions will actually be correct. That said, I have not tried an upgrade.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

getodk/docs#1655

Before submitting this PR, please make sure you have:

verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

matthew-white

I left a couple of random comments about .env, but I'm thinking that @lognaturel should take the first look at this PR.

matthew-white · 2023-09-18T03:42:34Z

.env.template

+# Optional: configure DKIM. Do not change these values.
+# DKIM_HOST_KEY=./files/dkim/rsa.private
+# DKIM_CONTAINER_KEY=/etc/exim4/dkim.key.temp


What do you think about moving these values closer to the EMAIL_* config? (Below them?) I feel like it's nice to keep thematically related config closer together.

matthew-white · 2023-09-18T03:50:30Z

.env.template

@@ -11,6 +11,10 @@ SSL_TYPE=letsencrypt
 HTTP_PORT=80
 HTTPS_PORT=443

+# Optional: configure DKIM. Do not change these values.


I probably missed the explanation, but just looking at this file, I feel unclear about whether these values are meant to configurable. If they're not configurable, I feel like we should try hard not to include them in this file, because it'd be one more thing for users to try to understand. Maybe there's something with variable defaults we could do in docker-compose.yml. (Defaults can be nested I think, if that helps.) On the other hand, if these values are configurable, I think we should soften the wording of "Do not change these values."

Another idea: maybe a DKIM_ENABLED variable that's true or false, similar to OIDC_ENABLED?

yanokwa · 2023-09-19T00:22:52Z

@lognaturel and I have found ways to move the variables out of docker-compose.

We've done this by creating an entirely new folder (mail, to match the container name) for the DKIM file. We'll create a zero byte rsa.private file by default so the volume binding in the container doesn't create an empty folder. Then during upgrade, we ask users to move their old files to this new location (see getodk/docs#1655).

We've updated .gitignore to make sure the rsa.p* files don't show up as dirty files in the repo, don't cause merge conflicts, and aren't accidentally checked into the repo. The latter two concerns are why we've moved to an entirely new folder structure instead of overwriting the old rsa.private.

I've sent in a PR to ix-ai/smtp#30 to only enable DKIM if the file has more than zero bytes, but I've also confirmed that mail sending works even if DKIM is enabled with an empty file. It sends, but includes an error message in the log.

Todo:

Verify that upgrading works with existing installs. I'm particularly concerned about Docker holding on to rsa.private and rsa.p* not being ignored via git.

yanokwa · 2023-09-20T06:03:21Z

I have verified this works on a clean install.

Fresh install with no DKIM.
Confirm email sending works with no DKIM (but with an error in EXIM log).
Run upgrade steps per Improve DKIM support docs#1655
Check out this PR.
Build and restart mail container per Improve DKIM support docs#1655
Enable DKIM per Improve DKIM support docs#1655
Confirm email sending works with DKIM (and no error)

I have verified this works on a install with a working DKIM, which admittedly is rare because someone would have to know to remove the config and rsa.private directories from the existing install and rebuild the mail container. But assuming that, I did the following:

Run upgrade steps per Improve DKIM support docs#1655
Check out this PR.
Build and restart mail container per Improve DKIM support docs#1655
Enable DKIM per Improve DKIM support docs#1655
Confirm email sending works with DKIM (and no error)

lognaturel · 2023-09-20T19:52:12Z

docker-compose.yml

    volumes:
-      - ./files/dkim/config:/etc/exim4/_docker_additional_macros:ro


Is this because it's considered there's no need for further configuration beyond the private key?

Correct. It's now in the upstream container: https://github.com/ix-ai/smtp/blob/master/entrypoint.sh#L61

config.disabled can be removed because those rules are now baked into the container.

lognaturel

I have talked extensively with @yanokwa about his process. I understand all the changes here and agree they make sense. I haven't tried it myself but I saw @yanokwa trying it.

yanokwa requested review from lognaturel and matthew-white September 17, 2023 23:19

matthew-white reviewed Sep 18, 2023

View reviewed changes

matthew-white assigned yanokwa Sep 18, 2023

matthew-white mentioned this pull request Sep 18, 2023

Update infrastructure dependencies for v2023.4 #440

Closed

yanokwa force-pushed the update-smtp-container branch 3 times, most recently from 41db24e to 5ccffc4 Compare September 19, 2023 00:17

yanokwa force-pushed the update-smtp-container branch 5 times, most recently from fd8fe6d to f740190 Compare September 19, 2023 23:40

Improve DKIM support

89f47c1

yanokwa force-pushed the update-smtp-container branch from f740190 to 89f47c1 Compare September 20, 2023 06:04

lognaturel reviewed Sep 20, 2023

View reviewed changes

lognaturel approved these changes Sep 20, 2023

View reviewed changes

matthew-white merged commit b7d384f into getodk:next Sep 21, 2023

yanokwa deleted the update-smtp-container branch October 6, 2024 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve DKIM support #485

Improve DKIM support #485

yanokwa commented Sep 17, 2023 •

edited by matthew-white

Loading

matthew-white left a comment

matthew-white Sep 18, 2023

matthew-white Sep 18, 2023

matthew-white Sep 18, 2023 •

edited

Loading

yanokwa commented Sep 19, 2023 •

edited

Loading

yanokwa commented Sep 20, 2023 •

edited

Loading

lognaturel Sep 20, 2023

yanokwa Sep 20, 2023 •

edited

Loading

lognaturel Sep 20, 2023

lognaturel left a comment

		volumes:
		- ./files/dkim/config:/etc/exim4/_docker_additional_macros:ro

Improve DKIM support #485

Improve DKIM support #485

Conversation

yanokwa commented Sep 17, 2023 • edited by matthew-white Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

matthew-white left a comment

Choose a reason for hiding this comment

matthew-white Sep 18, 2023

Choose a reason for hiding this comment

matthew-white Sep 18, 2023

Choose a reason for hiding this comment

matthew-white Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

yanokwa commented Sep 19, 2023 • edited Loading

yanokwa commented Sep 20, 2023 • edited Loading

lognaturel Sep 20, 2023

Choose a reason for hiding this comment

yanokwa Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

lognaturel Sep 20, 2023

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

yanokwa commented Sep 17, 2023 •

edited by matthew-white

Loading

matthew-white Sep 18, 2023 •

edited

Loading

yanokwa commented Sep 19, 2023 •

edited

Loading

yanokwa commented Sep 20, 2023 •

edited

Loading

yanokwa Sep 20, 2023 •

edited

Loading